rpmostreepayload: Set bootloader=none #2752

cgwalters · 2020-07-23T13:21:18Z

This matches
coreos/coreos-assembler@311768c

Since we're doing a new install, we're going to be installing an
updated bootloader (e.g. GRUB2) which should parse the BLS fragments
that OSTree generates. There's no reason for ostree to rerun
grub2-mkconfig.

pep8speaks · 2020-07-23T13:21:21Z

Hello @cgwalters! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-23 13:27:47 UTC

cgwalters · 2020-07-23T13:22:47Z

See https://lists.fedoraproject.org/archives/list/[email protected]/message/XVVAFHIK2XIOEHV4I6QOAG7DKSLI7E4V/

(Totally untested BTW, it's been a while since I've developed on Anaconda)

poncovka · 2020-07-23T14:08:55Z

Jenkins, it's ok to test.

jkonecny12 · 2020-07-27T08:29:21Z

Hi, @martinezjavier I think this could be an interesting change for you. So just a heads-up.

poncovka · 2020-07-28T10:16:38Z

Please, use https://github.com/coreos/coreos-assembler/commit/311768c in the commit message and the comment.

poncovka · 2020-07-28T11:18:46Z

I wasn't able to test it with Fedora-Silverblue-ostree-x86_64-Rawhide-20200721.n.0.iso because of the bug https://bugzilla.redhat.com/show_bug.cgi?id=1857530.

This matches coreos/coreos-assembler@311768c We don't need OSTree generating a new GRUB2 config everytime now since we embed a static GRUB2 config. Tell OSTree to just update the BLS entries using `sysroot.bootloader=none`. This gives a nice speedup as well at finalization time since we no longer call `grub2-mkconfig`. This only takes effect on new installs (here in Anaconda); since we're going to be installing an updated bootloader (e.g. GRUB2) which should parse the BLS fragments that OSTree generates.

cgwalters · 2020-07-28T12:24:29Z

Please, use coreos/coreos-assembler@311768c in the commit message and the comment.

Sure, done.

martinezjavier

The changes looks good to me!

VladimirSlavik

Code LGTM...

jkonecny12

Looks good to me too. Thanks for your contribution!

poncovka · 2020-08-03T13:26:31Z

Tested with Fedora-Silverblue-ostree-x86_64-Rawhide-20200802.n.0.iso. The installation is successful, the configuration is set up as expected, but the installed system doesn't boot (see the screenshot below).

cgwalters · 2020-08-03T13:45:06Z

Thanks a lot for testing this! I think the problem might be we were relying on the side effect of running grub2-mkconfig to generate the initial bootloader config. We probably need to change Anaconda to do the same thing we're doing in coreos-assembler where we just install the default GRUB config (although Anaconda may want to replace the boot device there? So perhaps we should just run grub-mkconfig...

Something like

diff --git a/pyanaconda/payload/rpmostreepayload.py b/pyanaconda/payload/rpmostreepayload.py
index b37e7081d..adfa61601 100644
--- a/pyanaconda/payload/rpmostreepayload.py
+++ b/pyanaconda/payload/rpmostreepayload.py
@@ -60,10 +60,6 @@ class RPMOSTreePayload(Payload):
         """The DBus type of the payload."""
         return PAYLOAD_TYPE_RPM_OSTREE
 
-    @property
-    def handles_bootloader_configuration(self):
-        return True
-
     @property
     def kernel_version_list(self):
         # OSTree handles bootloader configuration

?

AdamWill · 2020-08-20T22:47:46Z

FWIW, openQA actually relies surprisingly heavily on the 5 second grub timeout which anaconda writes to /etc/default/grub. grub's upstream default appears to be 1 second. Various openQA tests need to see the bootloader screen long enough to hit 'e' and edit the kernel command line. 1 second isn't enough, the test either doesn't see it or sees it but doesn't hit 'e' fast enough, either way it fails.

If this is merged in such a way that the installed system has a bootloader timeout of 1 second, it would be a problem for IoT testing, because several of the tests we run on IoT installs rely on this kernel command line editing. The other ostree installer image we test is Silverblue, but at a quick check, none of the tests we currently run on that image rely on this mechanism.

poncovka · 2020-09-01T12:11:38Z

Btw. I have tried the fix suggested in #2752 (comment), but the installed system still doesn't boot. Based on some warnings in the logs, I have tried the changes below, but it didn't help. Unfortunately, I don't understand this part well enough to be helpful.

diff --git a/pyanaconda/modules/payloads/base/utils.py b/pyanaconda/modules/payloads/base/utils.py
index 39c106989..a1865ad5f 100644
--- a/pyanaconda/modules/payloads/base/utils.py
+++ b/pyanaconda/modules/payloads/base/utils.py
@@ -95,10 +95,7 @@ def get_dir_size(directory):
 
 
 def get_kernel_version_list(root_path):
-    files = glob.glob(root_path + "/boot/vmlinuz-*")
-    files.extend(
-        glob.glob(root_path + "/boot/efi/EFI/{}/vmlinuz-*".format(conf.bootloader.efi_dir))
-    )
+    files = glob.glob(root_path + "/boot/**/vmlinuz-*", recursive=True)
 
     kernel_version_list = sorted((f.split("/")[-1][8:] for f in files
                                   if os.path.isfile(f) and "-rescue-" not in f),
diff --git a/pyanaconda/modules/storage/bootloader/grub2.py b/pyanaconda/modules/storage/bootloader/grub2.py
index e7b52aff1..d9f4c7cb5 100644
--- a/pyanaconda/modules/storage/bootloader/grub2.py
+++ b/pyanaconda/modules/storage/bootloader/grub2.py
@@ -343,7 +343,11 @@ class GRUB2(BootLoader):
             with open(machine_id_path, "r") as fd:
                 machine_id = fd.readline().strip()
 
-            default_entry = "%s-%s" % (machine_id, self.default.version)
+            if machine_id:
+                default_entry = "%s-%s" % (machine_id, self.default.version)
+            else:
+                default_entry = self.default.version
+
             rc = util.execInSysroot("grub2-set-default", [default_entry])
             if rc:
                 log.error("failed to set default menu entry to %s", productName)

diff --git a/pyanaconda/payload/rpmostreepayload.py b/pyanaconda/payload/rpmostreepayload.py
index b37e7081d..9435954c0 100644
--- a/pyanaconda/payload/rpmostreepayload.py
+++ b/pyanaconda/payload/rpmostreepayload.py
@@ -29,6 +29,7 @@ from pyanaconda.core.i18n import _
 from pyanaconda.modules.common.constants.objects import BOOTLOADER, DEVICE_TREE
 from pyanaconda.modules.common.constants.services import STORAGE
 from pyanaconda.modules.common.structures.storage import DeviceData
+from pyanaconda.modules.payloads.base.utils import get_kernel_version_list
 from pyanaconda.progress import progressQ
 from pyanaconda.payload.base import Payload
 from pyanaconda.payload import utils as payload_utils
@@ -60,14 +61,10 @@ class RPMOSTreePayload(Payload):
         """The DBus type of the payload."""
         return PAYLOAD_TYPE_RPM_OSTREE
 
-    @property
-    def handles_bootloader_configuration(self):
-        return True
-
     @property
     def kernel_version_list(self):
         # OSTree handles bootloader configuration
-        return []
+        return get_kernel_version_list(conf.target.system_root)
 
     @property
     def space_required(self):

cgwalters · 2021-03-05T15:58:39Z

If this is merged in such a way that the installed system has a bootloader timeout of 1 second, it would be a problem for IoT testing, because several of the tests we run on IoT installs rely on this kernel command line editing.

I think it's better for the test harness to manipulate the image offline for that; see e.g. coreos/coreos-assembler#1265

(There's also more recently work on Ignition kargs coreos/ignition#1168 )

jkonecny12 · 2021-03-11T10:53:47Z

Hi @cgwalters any idea why the last test still won't boot? It would be great to push this a bit.

github-actions · 2022-01-07T02:27:17Z

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

github-actions · 2022-02-07T02:09:34Z

This PR was closed because it has been stalled for 30 days with no activity.

cgwalters force-pushed the rpmostreepayload-bootloader-none branch from 957876a to f6aaa8d Compare July 23, 2020 13:27

poncovka added manual testing required This issue can't be merged without manual testing master Please, use the `f39` label instead. labels Jul 23, 2020

poncovka self-assigned this Jul 28, 2020

cgwalters force-pushed the rpmostreepayload-bootloader-none branch from f6aaa8d to 2f37ac6 Compare July 28, 2020 12:22

martinezjavier approved these changes Jul 28, 2020

View reviewed changes

VladimirSlavik approved these changes Jul 29, 2020

View reviewed changes

jkonecny12 approved these changes Jul 29, 2020

View reviewed changes

cgwalters mentioned this pull request Mar 5, 2021

Failed to update grub configuration if managing deployments on a loop device ostreedev/ostree#2289

Open

github-actions bot added the stale label Jan 7, 2022

github-actions bot closed this Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpmostreepayload: Set bootloader=none #2752

rpmostreepayload: Set bootloader=none #2752

cgwalters commented Jul 23, 2020

pep8speaks commented Jul 23, 2020 •

edited

Loading

cgwalters commented Jul 23, 2020

poncovka commented Jul 23, 2020

jkonecny12 commented Jul 27, 2020

poncovka commented Jul 28, 2020

poncovka commented Jul 28, 2020

cgwalters commented Jul 28, 2020

martinezjavier left a comment

VladimirSlavik left a comment

jkonecny12 left a comment

poncovka commented Aug 3, 2020

cgwalters commented Aug 3, 2020

AdamWill commented Aug 20, 2020

poncovka commented Sep 1, 2020

cgwalters commented Mar 5, 2021

jkonecny12 commented Mar 11, 2021

github-actions bot commented Jan 7, 2022

github-actions bot commented Feb 7, 2022

rpmostreepayload: Set bootloader=none #2752

rpmostreepayload: Set bootloader=none #2752

Conversation

cgwalters commented Jul 23, 2020

pep8speaks commented Jul 23, 2020 • edited Loading

Comment last updated at 2020-07-23 13:27:47 UTC

cgwalters commented Jul 23, 2020

poncovka commented Jul 23, 2020

jkonecny12 commented Jul 27, 2020

poncovka commented Jul 28, 2020

poncovka commented Jul 28, 2020

cgwalters commented Jul 28, 2020

martinezjavier left a comment

Choose a reason for hiding this comment

VladimirSlavik left a comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

poncovka commented Aug 3, 2020

cgwalters commented Aug 3, 2020

AdamWill commented Aug 20, 2020

poncovka commented Sep 1, 2020

cgwalters commented Mar 5, 2021

jkonecny12 commented Mar 11, 2021

github-actions bot commented Jan 7, 2022

github-actions bot commented Feb 7, 2022

pep8speaks commented Jul 23, 2020 •

edited

Loading